Repository navigation
Conversation
|
Updated 5:14 PM PT - Aug 25th, 2026
✅ @robobun, your commit 1f9b8bea7d06241636a0ebffb3353fe5cfb81258 passed in 🧪 To try this PR locally: bunx bun-pr 31032That installs a local version of the PR into your bun-31032 --bun |
WalkthroughThis PR contains two unrelated corrections to runtime and package manager code. The security scanner now guards unsafe refcount operations behind a success check, and the SSL configuration documentation clarifies the allocator contract for a helper function. ChangesSafety and documentation fixes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, fix, verification steps, and superseded security scanner change. It does not use the exact template headings, but it provides the required information and is mostly complete. Comment |
|
Closing per the no-test gate — this one genuinely can't be covered by a regression test:
The refcount trace in the PR body is the proof of correctness. Branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — verified start() self-derefs on Err (static_pipe_writer.rs:196/210), so gating the caller deref on is_ok() is correct and the Ok path is unchanged.
Extended reasoning...
Overview
Follow-up to #30875 addressing two inline review comments. In security_scanner.rs, wraps the post-start() RefCount::deref + started = false write in if start_result.is_ok(). In SSLConfig.rs, updates two stale doc-comment words (mi_free → free_sensitive, mimalloc → default-allocator) to match the already-updated block comment above them.
Verification
I checked src/spawn/static_pipe_writer.rs:175-228: start() does ref_() up front, and on the Err branch (both Windows :196 and POSIX :210) it self-deref()s and never sets started = true. So on Err the function is already refcount-neutral and started is already false — the caller's unconditional deref was indeed an over-deref, and the PR's refcount trace (2 → 3 → 2 → 1 → 0 → UAF/double-free via the scopeguard) is accurate. On Ok, the gated block runs exactly as before, so success-path behavior is byte-identical.
Security risks
None. No new inputs, no parsing, no auth/crypto surface. The change narrows when an unsafe deref runs; it cannot introduce a new unsafe path.
Level of scrutiny
This is intrusive-refcount code in unsafe Rust, which normally warrants care, but the diff is a 4-line conditional wrapper around existing code with no change to the common (Ok) path. Reachability of the fixed path is limited to PosixBufferedWriter::start failing on a fresh pipe fd (epoll/kqueue ENOMEM/ENOSPC), so risk of regression is very low. The SSLConfig changes are comment-only.
Other factors
This directly closes out two review-comment threads from #30875; cargo check and the security-scanner test suite pass per the description; no bugs were flagged by the bug-hunting pass.
4daf5f5 to
aa018cd
Compare
|
Reopened + rebased onto main (aa018cd onto 832edbc) per Discord — re-verified against current main:
No regression test: the Err path needs
|
There was a problem hiding this comment.
LGTM — verified StaticPipeWriter::start() self-derefs on Err (static_pipe_writer.rs:196/210), so gating the caller's deref on is_ok() is correct; Ok path is unchanged.
Extended reasoning...
Overview
Two changes, both follow-ups to review comments on #30875:
src/install/PackageManager/security_scanner.rs(finish_spawn): wraps the post-start()intrusivederef+started = falsereset inif start_result.is_ok(). I verified againstsrc/spawn/static_pipe_writer.rs:183/196/210thatstart()takes a+1at entry and self-releases it on theErrpath (both Windows and POSIX), leavingstarted == false. So the unconditional caller-side deref was indeed an over-deref onErr, and the refcount trace in the PR body (→ UAF in the scopeguard, double-free) is accurate. TheOkpath executes the exact same two statements as before, just inside theif— byte-for-byte identical happy-path behavior.src/runtime/socket/SSLConfig.rs: two doc-comment words updated (mi_free→free_sensitive,mimalloc→default-allocator) to match the adjacent block comment that already referencesbun_core::free_sensitive/default_alloc. Zero runtime impact.
Security risks
None. The refcount fix only affects an error path that requires epoll_ctl(ADD)/kevent to fail with ENOMEM/ENOSPC on a freshly-created socketpair fd — not attacker-reachable, and the change strictly removes a UAF/double-free rather than introducing one. The SSLConfig change is comment-only.
Level of scrutiny
Low. The diff is +8/-4 effective lines, the Ok path is provably unchanged, and the Err-path reasoning is mechanically verifiable against the start() source I checked. No CODEOWNERS entries cover either file.
Other factors
- Bug-hunting system found no issues; CodeRabbit found no actionable comments.
cargo checkand the security-scanner workspace test pass per the PR body / rebase comment.- The lack of a regression test is well-justified in the timeline (no fault-injection hook for the kernel error, and the
Okpath can't discriminate before/after). - Other
StaticPipeWriter::start()callers were already checked by the author (subprocess.rs:220 gates onstarted, shell/subproc.rs:891 doesn't extra-deref), so this was the only stale call site.
|
This over-deref was found again independently while working on the POSIX pipe writers, so I re-checked this PR against current main (b555e06), which includes #37774's rework of how
#37898 touches the same lines from the Windows side (it gates the scanner's release on the |
bun_core::dupe_z allocates with default_alloc::malloc and free_sensitive_cstr frees with default_alloc::free. Two doc comments in SSLConfig.rs still said mi_free and mimalloc. Update them to match the block comment above them. The security_scanner.rs change this PR first carried is already on main. #40478 returns early on the start() error before the deref, and the RefPtr locals release on Drop.
aa018cd to
1f9b8be
Compare
robobun
changed the title
|
Rebased onto main ( #40478 already fixed the over-deref on main. What remains is the |
There was a problem hiding this comment.
LGTM — comment-only fix aligning two stale doc lines with the block comment above them.
Checked that bun_core::dupe_z allocates via bun_alloc::default_alloc::malloc (util.rs:3685) and its own doc says it pairs with free_sensitive, so both wording changes are accurate.
Confirmed the diff touches only the two /// lines; no code, no behavior change.
CI failures (node-http-backpressure SIGKILL, WebSocket close timeout) are unrelated to a comment-only change.
Extended reasoning...
Overview
This PR edits two one-line doc comments on the zbox_into_raw and dupe_z helper functions in src/runtime/socket/SSLConfig.rs. It replaces mi_free with free_sensitive and "mimalloc allocation" with "default-allocator allocation". No executable code is touched. The security_scanner.rs refcount fix that this PR originally carried was superseded by #40478 and dropped on rebase; the remaining diff is exactly two comment lines.
Security risks
None. The change is documentation-only and does not alter allocation, freeing, or any control flow. The comments describe an existing allocator contract; the wording is being corrected to match what the code already does.
Level of scrutiny
Very low. I verified the factual claims directly: bun_core::dupe_z (src/bun_core/util.rs:3684) allocates with bun_alloc::default_alloc::malloc and its doc explicitly says it "Pairs with [free_sensitive]"; free_sensitive_cstr (src/bun_alloc/lib.rs:1027) frees via default_alloc::free. Under cfg(bun_asan) the default allocator is libc rather than mimalloc, so the old "mimalloc" / "mi_free" wording was indeed stale. The block comment immediately above these two lines (lines 52–59) already states the correct contract; this PR just brings the two per-function doc comments into agreement with it.
Other factors
No test is expected for a comment-only change. The CI failures on the older build are known flakes in unrelated HTTP tests and cannot be caused by comment edits. There are no prior reviews from me on this PR and no outstanding reviewer requests on the current revision.
|
Closing: comment-only change. |
Follow-up to #30875, requested in #30875 (comment).
Problem
src/runtime/socket/SSLConfig.rs(lines 61 and 67) name the wrong allocator. One saysmi_freefrees thedupe_zresult. The other saysdupe_zmakes "a fresh mimalloc allocation".bun_core::dupe_zallocates withbun_alloc::default_alloc::malloc(src/bun_core/util.rs:3685).free_sensitive_cstrfrees withdefault_alloc::free(src/bun_alloc/lib.rs:1038). Undercfg(bun_asan)that is libc, not mimalloc.Fix
free_sensitiveand "default-allocator allocation". No code changes.cargo check -p bun_runtimeis clean.bun bdbuilds and links.Background
SSLConfigowns C strings (keys, passphrases) that the HTTP layer frees. The allocation and the free must use the same allocator. The block comment at lines 52 to 59 documents this contract. The two comments below it are the ones this PR corrects.Notes
This PR first also carried a fix for
src/install/PackageManager/security_scanner.rs.finish_spawnreleased thestart()ref on both result arms, butStaticPipeWriter::start()already releases its own ref onErr. On theErrpath this freed the writer whilejson_writerstill pointed at it, and the cleanup guard then touched freed memory. #40478 (82123d3a61) fixed this on main:finish_spawnnow returns before the deref whenstart()fails, and theRefPtrlocals release onDrop. The rebase onto that commit dropped thesecurity_scanner.rshunk as superseded. Only the doc comment change remains.No regression test: the diff changes comments only.